HBASE-30142 Resolve NPE while running recoverUnknown command because null RegionLocation#8192
HBASE-30142 Resolve NPE while running recoverUnknown command because null RegionLocation#8192Umeshkumar9414 wants to merge 4 commits into
Conversation
wchevreuil
left a comment
There was a problem hiding this comment.
Makes sense. Should we add UT for this?
6e21a3a to
314f55e
Compare
@wchevreuil Added an test. Let me know if it looks good. |
There was a problem hiding this comment.
Pull request overview
Fixes HBASE-30142 by ensuring recoverUnknown/scheduleSCPsForUnknownServers does not treat regions with a null ServerName (i.e., null region location during transitions/failed-open handling) as “unknown servers,” avoiding spurious SCP scheduling and potential NPEs.
Changes:
- Update
ServerManager#isServerUnknownto returnfalsefornullserver names. - Add a new master-level mini-cluster test that drives a region into
ABNORMALLY_CLOSEDwith anulllocation and verifiesscheduleSCPsForUnknownServersschedules no SCPs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
hbase-server/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java |
Adjusts “unknown server” classification to ignore null server names and updates Javadoc. |
hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestRecoverUnknownWithNullRegionLocation.java |
Adds regression coverage for recoverUnknown when a region’s location is null but state is non-OFFLINE. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
314f55e to
4488d62
Compare
apurtell
left a comment
There was a problem hiding this comment.
Test classification needs a change, otherwise seems ok
4488d62 to
b501a6e
Compare
MasterRpcServices.scheduleSCPsForUnknownServers() has no null guard before calling isServerUnknown(). With the old code, regions with null serverName (OFFLINE/FAILED_OPEN in-between state) would spuriously trigger a ServerCrashProcedure. The fix prevents that.